Skip to content

Conversation

@kien-ngo
Copy link
Contributor

@kien-ngo kien-ngo commented Oct 3, 2024

Problem solved

Short description of the bug fixed or feature added


PR-Codex overview

This PR introduces the getSetClaimConditionPhases function to enhance the handling of claim conditions in the drops extension. It refactors the getMulticallSetClaimConditionTransactions function to utilize the new utility, improving code organization and reusability.

Detailed summary

  • Added getSetClaimConditionPhases function in get-set-claim-condition-phases.ts.
  • This function processes claim conditions and returns sorted phases and merkle information.
  • Updated getMulticallSetClaimConditionTransactions to call getSetClaimConditionPhases, removing redundant code.
  • Removed direct processing of phases and merkle roots from getMulticallSetClaimConditionTransactions.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@vercel
Copy link

vercel bot commented Oct 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2024 6:08pm
thirdweb_playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2024 6:08pm
thirdweb-www ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2024 6:08pm
wallet-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2024 6:08pm

@changeset-bot
Copy link

changeset-bot bot commented Oct 3, 2024

⚠️ No Changeset found

Latest commit: 9bcd0cc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
thirdweb (esm) 41.95 KB (0%) 840 ms (0%) 733 ms (+73.37% 🔺) 1.6 s
thirdweb (cjs) 101.63 KB (0%) 2.1 s (0%) 970 ms (-27.72% 🔽) 3.1 s
thirdweb (minimal + tree-shaking) 4.82 KB (0%) 97 ms (0%) 74 ms (+20.76% 🔺) 170 ms
thirdweb/chains (tree-shaking) 498 B (0%) 10 ms (0%) 17 ms (-48.47% 🔽) 27 ms
thirdweb/react (minimal + tree-shaking) 17.23 KB (0%) 345 ms (0%) 117 ms (-33.24% 🔽) 462 ms

* @param options
* @utils
*/
export async function getSetClaimConditionPhases(options: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this name is... very confusing.

I should be able to get a good idea of what this is for just by looking at the name.

Also i really dont think we should be exposing this this way. The inputs are kinda internal to us, you need to understand what tokenDecimals is for, what resetClaimEligibility does etc.

And once we expose it publicly, we can't change the API anymore.

Remind me again what's the use case you're trying to solve here? will help us figure out a good public API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do better here. Let me update the code and ping you again. This is a non-urgent issue

Copy link
Member

@joaquim-verges joaquim-verges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont want to expose this in this form, needs to be simplified

@kien-ngo kien-ngo closed this Oct 8, 2024
@kien-ngo kien-ngo deleted the kien/getSetClaimConditionPhases branch January 6, 2025 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DO NOT MERGE This pull request is still in progress and is not ready to be merged. packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants